Ignore received packets that weren't sent during examine_flow by Ryangwaite · Pull Request #13203 · sonic-net/sonic-mgmt

Ryangwaite · 2024-06-10T07:18:07Z

Attempts to process received packets that weren't sent resulted in a KeyError exception in the examine_flow disruption calculation. This commit ignores them.

Description of PR

Summary:
Fixes # (issue)

Microsoft ADO: 27806353

Type of change

Bug fix
Testbed and Framework(new/improvement)
Test case(new/improvement)

Back port request

Approach

What is the motivation for this PR?

The platform_tests.test_advanced_reboot.test_fast_reboot test was failing due to received packets that weren't sent.

How did you do it?

How did you verify/test it?

Modified the examine_flow function to run independently then tested the logic with a series of pcap files with various missing packets in the sequence. The cases included:

100 packets, happy path
100 packets, 0th packet receive missing
100 packets, 0th packet send missing
100 packets, 10th packet both missing, 11th packet both missing
100 packets, 10th packet both missing, 11th packet receive missing
100 packets, 10th packet both missing, 11th packet send missing
100 packets, 10th packet both missing
100 packets, 10th packet receive missing, 11th packet receive missing
100 packets, 10th packet receive missing, 11th packet send missing
100 packets, 10th packet receive missing
100 packets, 10th packet send missing, 11th packet receive missing
100 packets, 10th packet send missing, 11th packet send missing
100 packets, 10th packet send missing
100 packets, bunch of sad cases all in one:
- pkt-10 both missing
- pkt-15 send missing
- pkt-20 receive missing
- pkt-25 both missing, pkt-26 both missing
- pkt-30 both missing, pkt-31 send missing
- pkt-35 both missing, pkt-36 receive missing
- pkt-45 receive missing, pkt-46 receive missing
- pkt-55 receive missing, pkt-56 send missing
- pkt-65 send missing, pkt-66 send missing
- pkt-75 send missing, pkt-76 receive missing
100 packets, first 10 packets missing

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

ansible/roles/test/files/ptftests/py3/advanced-reboot.py

vaibhavhd · 2024-06-18T22:33:04Z

ansible/roles/test/files/ptftests/py3/advanced-reboot.py

This will miss the failure case - when there is a received packet drop and also a sent packet drop. We may not care if sent packets are not captured in pcap. But any drop in received packet should be reported.

Do not fail immediately upon noticing a received packet drop. We want to measure all received packet drops before concluding a failed state.

For the branch highlighted here, it handles the case where a received packet was present but no corresponding send packet in which case it logs it out, takes note of the received but not sent packet and carries on processing.

Further below, in the try-except while-loop it determines if packet IDs were missed for both send and receive from the pcaps and if so tracks them and continues processing.

Once all packets have been processed, if there were any that were missed from both send and received then these are logged out as a failure and the test will fail.

Attempts to process received packets that weren't sent resulted in a KeyError exception in the examine_flow disruption calculation. This commit ignores them and handles the case where there are missing packet ID's in the stream.

mssonicbld · 2024-06-24T05:18:56Z

The pre-commit check detected issues in the files touched by this pull request.
The pre-commit check is a mandatory check, please fix detected issues.

Detailed pre-commit check results:
trim trailing whitespace.................................................Passed fix end of files.........................................................Passed check yaml...........................................(no files to check)Skipped check for added large files..............................................Passed check python ast.........................................................Passed flake8...................................................................Failed - hook id: flake8 - exit code: 1 ansible/roles/test/files/ptftests/py3/advanced-reboot.py:2019:121: E501 line too long (128 > 120 characters) ansible/roles/test/files/ptftests/py3/advanced-reboot.py:2079:121: E501 line too long (139 > 120 characters) flake8...............................................(no files to check)Skipped check conditional mark sort..........................(no files to check)Skipped

To run the pre-commit checks locally, you can follow below steps:

Ensure that default python is python3. In sonic-mgmt docker container, default python is python2. You can run
the check by activating the python3 virtual environment in sonic-mgmt docker container or outside of sonic-mgmt
docker container.
Ensure that the pre-commit package is installed:

sudo pip install pre-commit

Go to repository root folder
Install the pre-commit hooks:

pre-commit install

Use pre-commit to check staged file:

pre-commit

Alternatively, you can check committed files using:

pre-commit run --from-ref <commit_id> --to-ref <commit_id>

We were initializing to 0 i.e. the first packet had been received before we've even checked for it. This meant a scenario where the first packet i.e. one with packet ID 0 was absent would be missed from the both send and receive packet absent list.

Ryangwaite · 2024-06-25T03:09:07Z

ansible/roles/test/files/ptftests/py3/advanced-reboot.py

        prev_payload = None
        if packets:
-            prev_payload, prev_time = 0, 0
+            prev_payload, prev_time = -1, 0


There was a problem where if the packet with packet id 0 (i.e. the first packet) was absent, it wouldn't be included in the analysis below. Changed to a non-packet ID by default to resolve this.

ansible/roles/test/files/ptftests/py3/advanced-reboot.py

Addresses PR comments.

StormLiangMS

LGTM

StormLiangMS · 2024-07-08T01:32:40Z

@Ryangwaite do we need to cherry pick to 202311 and 202405?

Ryangwaite · 2024-07-08T01:45:16Z

@Ryangwaite do we need to cherry pick to 202311 and 202405?

@StormLiangMS Yes please

…net#13203) What is the motivation for this PR? The platform_tests.test_advanced_reboot.test_fast_reboot test was failing due to received packets that weren't sent. How did you do it? How did you verify/test it? Modified the examine_flow function to run independently then tested the logic with a series of pcap files with various missing packets in the sequence. The cases included: 100 packets, happy path 100 packets, 0th packet receive missing 100 packets, 0th packet send missing 100 packets, 10th packet both missing, 11th packet both missing 100 packets, 10th packet both missing, 11th packet receive missing 100 packets, 10th packet both missing, 11th packet send missing 100 packets, 10th packet both missing 100 packets, 10th packet receive missing, 11th packet receive missing 100 packets, 10th packet receive missing, 11th packet send missing 100 packets, 10th packet receive missing 100 packets, 10th packet send missing, 11th packet receive missing 100 packets, 10th packet send missing, 11th packet send missing 100 packets, 10th packet send missing 100 packets, bunch of sad cases all in one: pkt-10 both missing pkt-15 send missing pkt-20 receive missing pkt-25 both missing, pkt-26 both missing pkt-30 both missing, pkt-31 send missing pkt-35 both missing, pkt-36 receive missing pkt-45 receive missing, pkt-46 receive missing pkt-55 receive missing, pkt-56 send missing pkt-65 send missing, pkt-66 send missing pkt-75 send missing, pkt-76 receive missing 100 packets, first 10 packets missing Any platform specific information?

mssonicbld · 2024-08-29T08:15:21Z

Cherry-pick PR to 202311: #14323

…net#13203) What is the motivation for this PR? The platform_tests.test_advanced_reboot.test_fast_reboot test was failing due to received packets that weren't sent. How did you do it? How did you verify/test it? Modified the examine_flow function to run independently then tested the logic with a series of pcap files with various missing packets in the sequence. The cases included: 100 packets, happy path 100 packets, 0th packet receive missing 100 packets, 0th packet send missing 100 packets, 10th packet both missing, 11th packet both missing 100 packets, 10th packet both missing, 11th packet receive missing 100 packets, 10th packet both missing, 11th packet send missing 100 packets, 10th packet both missing 100 packets, 10th packet receive missing, 11th packet receive missing 100 packets, 10th packet receive missing, 11th packet send missing 100 packets, 10th packet receive missing 100 packets, 10th packet send missing, 11th packet receive missing 100 packets, 10th packet send missing, 11th packet send missing 100 packets, 10th packet send missing 100 packets, bunch of sad cases all in one: pkt-10 both missing pkt-15 send missing pkt-20 receive missing pkt-25 both missing, pkt-26 both missing pkt-30 both missing, pkt-31 send missing pkt-35 both missing, pkt-36 receive missing pkt-45 receive missing, pkt-46 receive missing pkt-55 receive missing, pkt-56 send missing pkt-65 send missing, pkt-66 send missing pkt-75 send missing, pkt-76 receive missing 100 packets, first 10 packets missing Any platform specific information?

mssonicbld · 2024-08-29T08:15:28Z

Cherry-pick PR to 202405: #14324

What is the motivation for this PR? The platform_tests.test_advanced_reboot.test_fast_reboot test was failing due to received packets that weren't sent. How did you do it? How did you verify/test it? Modified the examine_flow function to run independently then tested the logic with a series of pcap files with various missing packets in the sequence. The cases included: 100 packets, happy path 100 packets, 0th packet receive missing 100 packets, 0th packet send missing 100 packets, 10th packet both missing, 11th packet both missing 100 packets, 10th packet both missing, 11th packet receive missing 100 packets, 10th packet both missing, 11th packet send missing 100 packets, 10th packet both missing 100 packets, 10th packet receive missing, 11th packet receive missing 100 packets, 10th packet receive missing, 11th packet send missing 100 packets, 10th packet receive missing 100 packets, 10th packet send missing, 11th packet receive missing 100 packets, 10th packet send missing, 11th packet send missing 100 packets, 10th packet send missing 100 packets, bunch of sad cases all in one: pkt-10 both missing pkt-15 send missing pkt-20 receive missing pkt-25 both missing, pkt-26 both missing pkt-30 both missing, pkt-31 send missing pkt-35 both missing, pkt-36 receive missing pkt-45 receive missing, pkt-46 receive missing pkt-55 receive missing, pkt-56 send missing pkt-65 send missing, pkt-66 send missing pkt-75 send missing, pkt-76 receive missing 100 packets, first 10 packets missing Any platform specific information?

…net#13203) What is the motivation for this PR? The platform_tests.test_advanced_reboot.test_fast_reboot test was failing due to received packets that weren't sent. How did you do it? How did you verify/test it? Modified the examine_flow function to run independently then tested the logic with a series of pcap files with various missing packets in the sequence. The cases included: 100 packets, happy path 100 packets, 0th packet receive missing 100 packets, 0th packet send missing 100 packets, 10th packet both missing, 11th packet both missing 100 packets, 10th packet both missing, 11th packet receive missing 100 packets, 10th packet both missing, 11th packet send missing 100 packets, 10th packet both missing 100 packets, 10th packet receive missing, 11th packet receive missing 100 packets, 10th packet receive missing, 11th packet send missing 100 packets, 10th packet receive missing 100 packets, 10th packet send missing, 11th packet receive missing 100 packets, 10th packet send missing, 11th packet send missing 100 packets, 10th packet send missing 100 packets, bunch of sad cases all in one: pkt-10 both missing pkt-15 send missing pkt-20 receive missing pkt-25 both missing, pkt-26 both missing pkt-30 both missing, pkt-31 send missing pkt-35 both missing, pkt-36 receive missing pkt-45 receive missing, pkt-46 receive missing pkt-55 receive missing, pkt-56 send missing pkt-65 send missing, pkt-66 send missing pkt-75 send missing, pkt-76 receive missing 100 packets, first 10 packets missing Any platform specific information?

vaibhavhd reviewed Jun 10, 2024

View reviewed changes

ansible/roles/test/files/ptftests/py3/advanced-reboot.py Outdated Show resolved Hide resolved

Ryangwaite force-pushed the advanced-reboot-ignore-not-sent-rcvd-pkts branch from 4ed775f to d372ee4 Compare June 14, 2024 07:05

vaibhavhd reviewed Jun 18, 2024

View reviewed changes

vaibhavhd requested a review from ryanzhu706 June 18, 2024 22:33

Ryangwaite force-pushed the advanced-reboot-ignore-not-sent-rcvd-pkts branch from d372ee4 to ba3b2b4 Compare June 24, 2024 05:17

Ryangwaite added 2 commits June 24, 2024 05:33

Improved logging of packets that were both sent and received

a4cbabb

Ryangwaite force-pushed the advanced-reboot-ignore-not-sent-rcvd-pkts branch from ba3b2b4 to 5056c12 Compare June 24, 2024 05:34

Ryangwaite commented Jun 25, 2024

View reviewed changes

Ryangwaite requested a review from vaibhavhd June 25, 2024 03:10

saiarcot895 reviewed Jul 2, 2024

View reviewed changes

ansible/roles/test/files/ptftests/py3/advanced-reboot.py Outdated Show resolved Hide resolved

Replaced try-catch with if-else + key in list check

6467d01

Addresses PR comments.

saiarcot895 approved these changes Jul 3, 2024

View reviewed changes

StormLiangMS approved these changes Jul 8, 2024

View reviewed changes

StormLiangMS merged commit 7aa6c67 into sonic-net:master Jul 8, 2024

vaibhavhd added Request for 202311 branch Request for 202405 branch labels Jul 10, 2024

StormLiangMS added Approved for 202311 branch Approved for 202405 branch labels Aug 29, 2024

mssonicbld added the Created PR to 202311 branch label Aug 29, 2024

mssonicbld mentioned this pull request Aug 29, 2024

[action] [PR:13203] Ignore received packets that weren't sent during examine_flow #14323

Merged

8 tasks

mssonicbld added the Created PR to 202405 branch label Aug 29, 2024

mssonicbld mentioned this pull request Aug 29, 2024

[action] [PR:13203] Ignore received packets that weren't sent during examine_flow #14324

Merged

8 tasks

mssonicbld added Included in 202311 branch and removed Created PR to 202311 branch labels Sep 2, 2024

mssonicbld added Included in 202405 branch and removed Created PR to 202405 branch labels Sep 2, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ignore received packets that weren't sent during examine_flow#13203

Ignore received packets that weren't sent during examine_flow#13203
StormLiangMS merged 4 commits intosonic-net:masterfrom
Ryangwaite:advanced-reboot-ignore-not-sent-rcvd-pkts

Ryangwaite commented Jun 10, 2024 •

edited

Loading

Uh oh!

Uh oh!

vaibhavhd Jun 18, 2024

Uh oh!

Ryangwaite Jun 24, 2024 •

edited

Loading

Uh oh!

mssonicbld commented Jun 24, 2024

Uh oh!

Ryangwaite Jun 25, 2024

Uh oh!

Uh oh!

StormLiangMS left a comment

Uh oh!

StormLiangMS commented Jul 8, 2024

Uh oh!

Ryangwaite commented Jul 8, 2024 •

edited

Loading

Uh oh!

mssonicbld commented Aug 29, 2024

Uh oh!

mssonicbld commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

Ryangwaite commented Jun 10, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of PR

Type of change

Back port request

Approach

What is the motivation for this PR?

How did you do it?

How did you verify/test it?

Any platform specific information?

Supported testbed topology if it's a new test case?

Documentation

Uh oh!

Uh oh!

vaibhavhd Jun 18, 2024

Choose a reason for hiding this comment

Uh oh!

Ryangwaite Jun 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mssonicbld commented Jun 24, 2024

Uh oh!

Ryangwaite Jun 25, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

StormLiangMS left a comment

Choose a reason for hiding this comment

Uh oh!

StormLiangMS commented Jul 8, 2024

Uh oh!

Ryangwaite commented Jul 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mssonicbld commented Aug 29, 2024

Uh oh!

mssonicbld commented Aug 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Ryangwaite commented Jun 10, 2024 •

edited

Loading

Ryangwaite Jun 24, 2024 •

edited

Loading

Ryangwaite commented Jul 8, 2024 •

edited

Loading